Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

http: do not replace .read() in IncomingMessage #18939

Closed
wants to merge 1 commit into from
Closed

http: do not replace .read() in IncomingMessage #18939

wants to merge 1 commit into from

Conversation

mcollina
Copy link
Member

This removes some monkeypatching in IncomingMessage.prototype.read() that I did not understand. Surprisingly, none of our tests fails if I remove them.

I'm opening this PR mainly to gather understanding of what these lines of code do, and if we really need those.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

http

@nodejs-github-bot nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Feb 22, 2018
@mcollina
Copy link
Member Author

CI: https://ci.nodejs.org/job/node-test-pull-request/13334/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1305/

@mcollina mcollina mentioned this pull request Feb 22, 2018
Closed
addaleax
addaleax approved these changes Feb 22, 2018
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_consuming is used in _http_server.js as well, I guess you could change that now that it has become a constant?

@mcollina
Copy link
Member Author

Very interestling, this test is failing on some machines:

not ok 702 parallel/test-http-connect

duration_ms: 0.631
severity: fail
stack: |-
assert.js:74
throw new AssertionError(obj);
^

AssertionError [ERR_ASSERTION]: 2 strictEqual 1
    at Server.server.on.common.mustCall (/home/iojs/build/workspace/node-test-commit-smartos/nodes/smartos15-64/test/parallel/test-http-connect.js:37:10)
    at Server.<anonymous> (/home/iojs/build/workspace/node-test-commit-smartos/nodes/smartos15-64/test/common/index.js:476:15)
    at Server.emit (events.js:129:13)
    at onParserExecuteCommon (_http_server.js:536:14)
    at onParserExecute (_http_server.js:483:3)

@mcollina
Copy link
Member Author

Seems it’s a flaky test #18940. I’ll rebased and rerun later today.

@addaleax
Copy link
Member

addaleax commented Feb 22, 2018 •
edited
Loading

@mcollina That’s unrelated and coming from master, see #18940 :) A new CI should look better.

Edit: Jinx. 😄

@fhinkel
Copy link
Member

fhinkel commented Feb 22, 2018 •
edited
Loading

After looking at this more carefully, I'm in favor of removing it. It will run through CITGM, right?

@mcollina mcollina force-pushed the no-monkey-patching-read-incoming-message branch from f1452f8 to a5e23ab Compare February 22, 2018 22:50
@mcollina
Copy link
Member Author

I've tracked down where _consuming is used. The logic is to dump any body that has not been processed when a response is sent. Unluckily, this functionality is not tested at all.

I will add a test for it and come up with an alternative fix that maintain the functionality.

The logic was added in 8624adf and 1d36931.

@mcollina
Copy link
Member Author

An alternative approach is to remove the logic completely.

@BridgeAR
Copy link
Member

It is of course difficult to tell if _consuming was ever used in the wild but since we do not have a unit test for it, it seems fine for me to remove it?

To me it feels like we should just make this a semver-major to be on the safe side. If others feel different, please just comment.

@BridgeAR BridgeAR added the semver-major PRs that contain breaking changes and should be released in the next major version. label Feb 23, 2018
BridgeAR
BridgeAR approved these changes Feb 23, 2018
View reviewed changes
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice finding.

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 23, 2018
@mcollina mcollina force-pushed the no-monkey-patching-read-incoming-message branch from a5e23ab to 1bbac5d Compare February 26, 2018 10:05
@mcollina
Copy link
Member Author

CI: https://ci.nodejs.org/job/node-test-pull-request/13371/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1310/

Confirmed, that block of code is superseded and it can be removed safely.

@mcollina
Copy link
Member Author

@fhinkel @addaleax may I get a confirmation of your LGTMs, this can land.

@addaleax
Copy link
Member

@mcollina Yes, this still LGTM

mcollina added a commit that referenced this pull request Feb 27, 2018
@mcollina
http: do not replace .read() in IncomingMessage
29be1e5 
Remove the req._consumed property, as its use is completely
superseded and not needed anymore. This was being set in the
overridden .read().

PR-URL: #18939
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@mcollina
Copy link
Member Author

Landed as 29be1e5.

@mcollina mcollina closed this Feb 27, 2018
@mcollina mcollina deleted the no-monkey-patching-read-incoming-message branch February 27, 2018 18:06
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
@mcollina @MayaLekova
http: do not replace .read() in IncomingMessage
4b28d67 
Remove the req._consumed property, as its use is completely
superseded and not needed anymore. This was being set in the
overridden .read().

PR-URL: nodejs#18939
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fhinkel fhinkel fhinkel approved these changes

@jasnell jasnell jasnell approved these changes

@addaleax addaleax addaleax approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. http Issues or PRs related to the http subsystem. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@mcollina @addaleax @fhinkel @BridgeAR @jasnell @nodejs-github-bot